Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: create-dojo project broken unless name is worlds #333

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xJonathanLEI
Copy link

@xJonathanLEI xJonathanLEI commented Nov 17, 2024

Currently the create-dojo command is broken in 2 ways:

  1. It attempts to clone the dojo-starter repository twice. The second attempt always fails, but the project creation process is not interrupted thanks to the command result not being checked. Nonetheless it's bad DX.
  2. The examples reference the dojo-starter folder by ../../worlds/dojo-starter, which means newly created projects must be named exactly worlds to work. This is the bigger issue.

Introduced changes

  • Made examples' references to the dojo-starter folder compatible with a newly created project by adding a symlink
  • Removed the double-cloning of dojo-starter.

Summary by CodeRabbit

  • New Features

    • Introduced a new directory dojo-starter for future content organization.
    • Updated the cloning mechanism for the dojo-starter repository to use npx degit for improved efficiency.
  • Bug Fixes

    • Enhanced error handling and console log messages related to the cloning process.
  • Documentation

    • Updated import paths in multiple dojoConfig.ts files for clarity and simplicity.

Copy link

coderabbitai bot commented Nov 17, 2024

Walkthrough

A new directory named dojo-starter has been added under the worlds directory, serving as a structural enhancement without any file modifications or changes to existing logic. Additionally, the import paths for the manifest variable in multiple dojoConfig.ts files have been simplified from a two-level path to a one-level path, reflecting a reorganization of the directory structure. The start.ts file has also been updated to change the cloning method of the dojo-starter repository from git clone to npx degit, with adjustments to related error handling and logging.

Changes

File Path Change Summary
examples/dojo-starter New directory added under worlds.
examples/example-nodejs-bot/dojoConfig.ts Updated import path for manifest from "../../worlds/dojo-starter/manifest_dev.json" to "../dojo-starter/manifest_dev.json".
examples/example-vanillajs-phaser-recs/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vite-kitchen-sink/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vite-react-app-recs/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vite-react-phaser-recs/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vite-react-pwa-recs/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vite-react-sdk/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vite-react-threejs-recs/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vite-svelte-recs/dojoConfig.ts Updated import path for manifest similarly as above.
examples/example-vue-app-recs/dojoConfig.ts Updated import path for manifest similarly as above.
packages/create-dojo/src/commands/start.ts Changed cloning method from git clone to npx degit, updated variable names and error handling.

Possibly related PRs

  • fix: docs and example #231: Involves changes to the dojo-starter subproject, directly related to the structural changes made in the main PR.
  • fix: lock submodule #243: Updates the dojo-starter submodule, indicating a connection to the structural changes in the main PR.
  • fix: wasm build issues #256: Reflects another update to the dojo-starter subproject, reinforcing the relationship with the main PR's changes.
  • fix: submodule #272: Modifies the path of the dojo-starter submodule, relevant to the structural changes introduced in the main PR.
  • fix: paths #311: Updates import paths for the manifest variable in several dojoConfig.ts files, directly related to the changes made in the main PR.
  • feat: fix for create-dojo cli #312: Addresses issues related to CLI functionality, which may involve the dojo-starter structure indirectly.
  • fix: bumps #326: Includes updates to the CI workflow that may be affected by the structural changes in the main PR, particularly regarding the handling of the dojo-starter directory.

Suggested reviewers

  • ponderingdemocritus

🐇 In the world of code, a new path we tread,
With dojo-starter added, new adventures ahead.
Import paths simplified, like a hop through the grass,
With npx degit now, the old ways have passed.
So let’s celebrate this change with a cheer,
For a brighter, cleaner codebase, let’s give a big ear! 🎉


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
packages/create-dojo/src/commands/start.ts (3)

69-79: Enhance error handling and robustness of the cloning operation.

While the switch to npx degit is a good improvement, consider these enhancements:

  1. Add a check for npx availability
  2. Provide more detailed error messages to help users troubleshoot

Consider this implementation:

     // Clone dojo-starter
     console.log(`Downloading dojo-starter...`);
+    // Check if npx is available
+    const npxCheck = spawn.sync('npx', ['--version'], { stdio: 'ignore' });
+    if (npxCheck.status !== 0) {
+        throw new Error('npx is required but not found. Please install [email protected] or later.');
+    }
+
     const starterCloneResult = spawn.sync(
         "npx",
         ["degit", `dojoengine/dojo-starter`, dojoStarterPath],
         { stdio: "inherit" }
     );

     if (starterCloneResult.status !== 0) {
-        throw new Error(`Failed to clone dojo-starter repository.`);
+        throw new Error(
+            `Failed to clone dojo-starter repository. ` +
+            `Please ensure you have internet connectivity and the repository exists at ` +
+            `https://github.com/dojoengine/dojo-starter`
+        );
     }

Line range hint 39-46: Review directory structure for path compatibility.

The current implementation creates dojo-starter inside the project directory, but the PR mentions issues with relative paths in examples referring to ../../worlds/dojo-starter. This structure might still cause issues with example configurations.

Consider these options:

  1. Update all example configurations to use relative paths based on the new structure
  2. Create a symlink from ../../worlds/dojo-starter to the local dojo-starter directory
  3. Document the expected directory structure in the README

Would you like me to help implement any of these solutions?


Line range hint 1-8: Enhance template validation and error handling.

The template validation could be improved to:

  1. Validate template existence before starting the initialization
  2. Handle network issues when fetching templates
  3. Provide better error messages for template-related issues

Consider enhancing the template validation:

     .action(async (options) => {
         try {
             const cwd = path.resolve(options.cwd);
             let template: string;
+            
+            // Validate template repository existence
+            const validateTemplate = async (templateName: string) => {
+                const url = `https://api.github.com/repos/dojoengine/dojo.js/contents/examples/${templateName}`;
+                try {
+                    const response = await fetch(url);
+                    return response.status === 200;
+                } catch (error) {
+                    return false;
+                }
+            };

             if (options.template) {
                 const selectedTemplate = templates.find(
                     (tpl) => tpl.value === options.template
                 );
                 if (!selectedTemplate) {
                     console.error(
-                        `Template "${options.template}" not found. Available templates are: ${templates
+                        `Template "${options.template}" not found.\n\nAvailable templates:\n${templates
+                            .map((tpl) => `- ${tpl.value}: ${tpl.description}`)
+                            .join("\n")}`
-                            .map((tpl) => tpl.value)
-                            .join(", ")}`
                     );
                     process.exit(1);
                 }
+                if (!(await validateTemplate(selectedTemplate.value))) {
+                    throw new Error(
+                        `Template "${selectedTemplate.value}" exists in the list but ` +
+                        `could not be accessed. Please check your internet connection ` +
+                        `or report this issue.`
+                    );
+                }
                 template = selectedTemplate.value;
             }

Also applies to: 144-167

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b47c945 and 18eadc5.

📒 Files selected for processing (12)
  • examples/dojo-starter (1 hunks)
  • examples/example-nodejs-bot/dojoConfig.ts (1 hunks)
  • examples/example-vanillajs-phaser-recs/dojoConfig.ts (1 hunks)
  • examples/example-vite-kitchen-sink/dojoConfig.ts (1 hunks)
  • examples/example-vite-react-app-recs/dojoConfig.ts (1 hunks)
  • examples/example-vite-react-phaser-recs/dojoConfig.ts (1 hunks)
  • examples/example-vite-react-pwa-recs/dojoConfig.ts (1 hunks)
  • examples/example-vite-react-sdk/dojoConfig.ts (1 hunks)
  • examples/example-vite-react-threejs-recs/dojoConfig.ts (1 hunks)
  • examples/example-vite-svelte-recs/dojoConfig.ts (1 hunks)
  • examples/example-vue-app-recs/dojoConfig.ts (1 hunks)
  • packages/create-dojo/src/commands/start.ts (1 hunks)
✅ Files skipped from review due to trivial changes (11)
  • examples/dojo-starter
  • examples/example-nodejs-bot/dojoConfig.ts
  • examples/example-vanillajs-phaser-recs/dojoConfig.ts
  • examples/example-vite-kitchen-sink/dojoConfig.ts
  • examples/example-vite-react-app-recs/dojoConfig.ts
  • examples/example-vite-react-phaser-recs/dojoConfig.ts
  • examples/example-vite-react-pwa-recs/dojoConfig.ts
  • examples/example-vite-react-sdk/dojoConfig.ts
  • examples/example-vite-react-threejs-recs/dojoConfig.ts
  • examples/example-vite-svelte-recs/dojoConfig.ts
  • examples/example-vue-app-recs/dojoConfig.ts
🔇 Additional comments (1)
packages/create-dojo/src/commands/start.ts (1)

Line range hint 39-46: Verify directory structure and symlink creation.

According to the PR objectives, a symlink should be created to ensure compatibility with newly created projects. However, I don't see the symlink creation in the code.

Let's verify the directory structure and symlink requirements:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant